Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Decouple preference window from wallpaperController (Requires #141) #144

Merged
merged 3 commits into from
Feb 20, 2023

Conversation

Lucki
Copy link
Contributor

@Lucki Lucki commented Dec 12, 2022

I've decoupled the preferences window from all background activity. The background controller now tracks some gsetting changes to communicate with the preferences window. I'm confident this fixes #135 since there's now only one single wallpaperController running.

This also fixes #126 - just use your favorite keyboard shortcut manager (like gnomes in settings -> keyboard -> shortcuts -> own shortcuts) to set the variable request-new-wallpaper to true:

gsettings --schemadir "${XDG_DATA_HOME:-$HOME/.local/share}/gnome-shell/extensions/[email protected]/schemas" set org.gnome.shell.extensions.space.iflow.randomwallpaper.backend-connection request-new-wallpaper true

@ifl0w ifl0w changed the base branch from develop to WIP_v3.0.0 December 14, 2022 21:46
@ifl0w
Copy link
Owner

ifl0w commented Dec 14, 2022

@Lucki would you be so kind to git rebase this PR on the WIP_3.0.0 branch. Not sure if this is handled automatically by git since the commit hashes are different. You might have to drop the first three commits manually to remove the duplicate/conflicting commits from the PR.

Sorry for the extra effort!

@Lucki
Copy link
Contributor Author

Lucki commented Dec 14, 2022

All done! GitHub got this correctly and no commit dropping was required. Make sure you visit #141 first!

@ifl0w ifl0w self-requested a review February 16, 2023 23:50
Copy link
Owner

@ifl0w ifl0w left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks agian! This is a very interesting and exciting change! Is it good practice to use gschemas as IPC mechanism, though?

Generally, I'm fine with it and interested in how that works out! Also, a cool side effect is that now the wallpaper change can be triggered from other applications/languages as well!

I only added a small comment.

[email protected]/wallpaperController.js Outdated Show resolved Hide resolved
@Lucki
Copy link
Contributor Author

Lucki commented Feb 17, 2023

Is it good practice to use gschemas as IPC mechanism, though?

I honestly don't know but since you already had the observer mechanism in place it wasn't far to this.

@ifl0w
Copy link
Owner

ifl0w commented Feb 19, 2023

I honestly don't know but since you already had the observer mechanism in place it wasn't far to this.

Yeah, let's try and see how this works out! The only thing I want to avoid is accepting some (potentially unsafe) user input via this backend-connection.

@ifl0w ifl0w self-requested a review February 19, 2023 17:56
@ifl0w
Copy link
Owner

ifl0w commented Feb 19, 2023

@Lucki Again, could you please rebase the commit? Sorry for that!
If this is too annoying to you, I could pull in your changes directly without handling the PR in the GitHub UI, but I honestly don't know if I can make the GitHub interface correctly pick up the manual changes.

@Lucki
Copy link
Contributor Author

Lucki commented Feb 19, 2023

I want to avoid is accepting some (potentially unsafe) user input via this backend-connection.

There are only simple true/false toggles for:

  • Clearing the history
  • Pausing the timer
  • Requesting new wallpapers
  • Opening the wallpaper folder

No user input directly.


Hm, strange that this doesn't work automatic. It worked automatic locally and GitHub doesn't even register anything different in the force push…

Guess I have to rebase every PR when the previous one is merged then.

@ifl0w
Copy link
Owner

ifl0w commented Feb 20, 2023

Yeah, exactly; these boolean triggers won't be an issue at all. I just realized that I have to look at all the other schemas in general and see what exactly is stored there and how it is used because I never thought about it a lot.

Yeah, the hashes change once it is merged and it is a bit annoying that GH does not recognize this.

@ifl0w ifl0w merged commit 360677f into ifl0w:WIP_v3.0.0 Feb 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Auto-fetch is downloading multiple images Add a keyboard shortcut for changing the wallpaper
2 participants